[GitHub] [airflow] hankehly commented on a diff in pull request #22804: Support log download in task log view

[GitBox <gi...@apache.org>]

hankehly commented on code in PR #22804:
URL: https://github.com/apache/airflow/pull/22804#discussion_r844638924


##########
airflow/www/templates/airflow/ti_log.html:
##########
@@ -53,6 +53,7 @@ <h4>{{ title }}</h4>
     <div class="col-md-4 text-right">
       <a class="btn btn-default" onclick="scrollBottomLogs()">Jump To End</a>
       <a class="btn btn-default" onclick="toggleWrapLogs()">Toggle Wrap</a>
+      <a class="btn btn-default" onclick="downloadActiveLog()">Download</a>

Review Comment:
   @uranusjr Thanks for the suggestion. We can definitely do that.
   
   It looks like the active "try number" is managed on the frontend, so one would still need to use javascript to render the correct url. An update might look like this:
   
   #### ti_log.html
   ```html
   <a class="btn btn-default" id="ti_log_download_active" download>Download</a>
   ```
   
   #### ti_log.js
   ```js
   function setDownloadUrl(tryNumber) {
     if (!tryNumber) {
       // default to the active tab value
       tryNumber = $('#ti_log_try_index_list .active a').attr('href').replace(/\D/g,'');
     }
     const query = new URLSearchParams({
       dag_id: dagId,
       task_id: taskId,
       execution_date: executionDate,
       try_number: tryNumber,
       metadata: 'null',
       format: 'file'
     });
     const url = `${logsWithMetadataUrl}?${query}`;
     $('#ti_log_download_active').attr('href', url);
   }
   
   $(document).ready(() => {
     // Set the download link once on initial page load
     setDownloadUrl();
     $('#ti_log_try_index_list a').click(function () {
       // Set it again after clicking a new try_number.
       // Obtain try_number from the clicked element because the
       // .active class has not been replaced yet.
       const tryNumber = $(this).attr('href').replace(/\D/g,'');
       setDownloadUrl(tryNumber);
     });
   });
   ```
   
   Regarding the `target` or `download` attributes, I chose to use neither for the following reasons:
   - the log download links in the graph view task modal do not use them
   - the flask handler sets the `Content-Disposition` header with the `attachment` parameter
   - `download` doesn't seem to be [supported on IE](https://developer.mozilla.org/en-US/docs/Web/HTML/Element/a#browser_compatibility)
   
   Please let me know your thoughts, I'd be happy to make changes.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org